Skip to content

fix: 9 - add sanitization of cli inputs in launch_explorer.py - #776

Merged
daniel-rdt merged 14 commits into
masterfrom
fix/security-fix-9
Jul 9, 2026
Merged

fix: 9 - add sanitization of cli inputs in launch_explorer.py#776
daniel-rdt merged 14 commits into
masterfrom
fix/security-fix-9

Conversation

@daniel-rdt

@daniel-rdt daniel-rdt commented Jul 2, 2026

Copy link
Copy Markdown
Member

Changes proposed in this Pull Request

This PR sanitizes the handling of CLI inputs passed to launch_explorer.py where untrusted sys.argv values used to flow unchecked into logging.FileHandler (file write) and pypsa.Network (file read).

All three are applied before any filesystem access; invalid input raises ValueError and aborts. The Snakemake inputs are untouched, since its values come from trusted workflow config.

Workflow

  • (new) functionsanitize_path — allow-lists characters, rejects absolute paths and .. traversal, and requires the resolved path to stay within the working directory.
  • (new) function sanitize_port — accepts only integers in 1024–65535 to account for privileged ports and protocol limits.
  • (new) function sanitize_input_files — validates each path, requiring an existing .nc file.

Generative AI was used to assist with code implementation of this PR. All content has been reviewed, verified, and is the responsibility of the author.

Checklist

  • Security scan(s) show no high-severity bugs, critical vulnerabilities, or exposed secrets.
  • I tested my contribution locally and it works as intended.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to pixi.toml (using pixi add <dependency-name>).
  • Changes in configuration options are added in config/config.default.yaml.
  • Changes in configuration options are documented in doc/configtables/*.csv.
  • Changes in configuration options are added in config/test/*.yaml.
  • The multiple weather/climate years test is passing locally (using pixi run -e open-tyndp tyndp-cyears-test).
  • Open-TYNDP SPDX license header added to all touched files.
  • For new data sources or versions, these instructions have been followed.
  • New rules are documented in the appropriate doc/*.rst files.
  • A release note doc/release_notes.rst is added.
  • Major features are documented with up-to-date information in doc/index.rst.
  • Module docstrings added to new Python scripts.

@daniel-rdt daniel-rdt self-assigned this Jul 2, 2026
@daniel-rdt daniel-rdt added SB Scenario Building CBA Cost Benefit Analysis labels Jul 2, 2026
@daniel-rdt
daniel-rdt changed the base branch from scan-branch to master July 2, 2026 13:09
@daniel-rdt
daniel-rdt changed the base branch from master to scan-branch July 2, 2026 13:10
@daniel-rdt
daniel-rdt marked this pull request as ready for review July 2, 2026 13:10
@daniel-rdt
daniel-rdt requested a review from tgilon July 2, 2026 13:10
@tgilon
tgilon changed the base branch from scan-branch to master July 2, 2026 14:05

@tgilon tgilon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @daniel-rdt for this addition! I have a few comments but would like to ask @KristijanFaust-OET if he has any recommendations. This PR improves input validation, feel free to suggest improvements.

Comment thread doc/release_notes.rst Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread doc/release_notes.rst Outdated

@KristijanFaust-OET KristijanFaust-OET left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments which are more questions at the moment than suggestions. Besides that, do we have any unit testing which can be extended with this validation cases?

Comment thread scripts/sb/launch_explorer.py Outdated
Comment thread scripts/sb/launch_explorer.py
@tgilon tgilon added security and removed SB Scenario Building CBA Cost Benefit Analysis labels Jul 6, 2026
Comment thread scripts/sb/launch_explorer.py Outdated
@daniel-rdt

Copy link
Copy Markdown
Member Author

Thank you @tgilon and @KristijanFaust-OET for the review and suggestions. I made the simplifications and improvements as suggested above. Let me know if this works better for you.

# Conflicts:
#	doc/release_notes.md

@tgilon tgilon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good now. Could you please update the doc accordingly? I will then approve! Thank you @daniel-rdt

Comment thread doc/release_notes.md Outdated
Comment thread scripts/sb/launch_explorer.py
Comment thread scripts/sb/launch_explorer.py Outdated
daniel-rdt and others added 2 commits July 9, 2026 13:29
Co-authored-by: Thomas Gilon <thomas.gilon@openenergytransition.org>
@daniel-rdt
daniel-rdt requested a review from tgilon July 9, 2026 11:34
@daniel-rdt
daniel-rdt merged commit 1fb42f8 into master Jul 9, 2026
7 checks passed
@daniel-rdt
daniel-rdt deleted the fix/security-fix-9 branch July 9, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants